Upgrade protobuf to match with tf-nightly#170
Conversation
dryman
left a comment
dryman
left a comment
There was a problem hiding this comment.
LGTM.
I'm creating an internal PR without the std::optional -> absl::optional changes.
| // Default: `absl::nullopt`. | ||
| Options& set_window_log(std::optional<int> window_log) { | ||
| Options& set_window_log(absl::optional<int> window_log) { | ||
| compressor_options_.set_window_log(window_log); |
There was a problem hiding this comment.
The upstream riegeli package uses std::optional instead of absl::optional.
https://github.com/google/riegeli/blob/0895cfaf4787a03afeed86ddacba3dc9a8beae52/riegeli/brotli/brotli_writer.h#L87
So I prefer to use the same convention.
There was a problem hiding this comment.
Sure! I did it because for some reason on my local macos arm setup this was causing a compilation error.
There was a problem hiding this comment.
This may due to two reasons (either or, or both)
- The compiler didn't recognize c++17 (I think this is less likely because we already specify --cxxopt=std=c++17)
- Linking incorrect stdlib (in some platforms, there are separated libstdc++)
You might find you installed libstdc++ on your platform which didn't support c++17, but the binary linked to it.
|
FYI, Ihor is out of office. While this PR LGTM, I need to sync with him to know when we can merge so that we don't break the pygrain and TF ecosystem. |
mtsokol
changed the title
|
Now we use the released TF, rather than |
2 participants
Hi @dryman,
This cleanup can replace #168. I upgrade protobuf to the exact TF version here. I'd need an opinion on Beam deps update here - there are no tests for Beam module.